fix(k8s): make helm test pods schedulable and probe CubeProxy healthz - #1388

Review: fix(k8s): make helm test pods schedulable and probe CubeProxy healthz (#1388)

AI-generated review — this is a static analysis of the diff against the base branch; it is not a human approval.

Verdict

The change is well-scoped and directly addresses the #1272 review blockers. The placement model is right: cube.testPlacement (union of control+compute tolerations, no nodeSelector) is what actually unsticks Pending test pods on tainted nodes without pinning to a plane the other topology doesn't have, and keeping node-runtime-test on cube.computePlacement is correct since the hostPath sockets only exist on cube-node hosts. The proxy-control-test rewrite to probe /admin/healthz with the admin token is sound, and I verified the wiring end to end:

• the release Secret always carries cube-admin-token and exists whenever cube.proxyEnabled is true (templates/secret.yaml:37, secretEnabled includes proxy);
• the proxy Service exposes admin with targetPort: admin (proxy-service.yaml:40-46) and the Deployment declares a matching container port named admin (proxy.yaml:259-261);
• the proxy already consumes the same token from the same secret key and probes the same /admin/healthz + X-Cube-Admin-Token pattern in its readiness/liveness (proxy.yaml:200-204, 268, 279), so the helm test now mirrors the real admin contract instead of the dataplane / (which correctly returns 400).

The new guard script (test-helm-test-guards.sh) is high-quality and is wired into CI: .github/workflows/kubernetes-chart-check.yml:68 globs deploy/kubernetes/*/scripts/test-*.sh, so it runs on every chart PR. I traced all six render modes (default, control-only, compute-only, single-node, custom-taint, disabled) against templates/validate.yaml and the default/values-single-node.yaml values — all pass validation. The Python assertions also match what Helm actually renders: the toleration-key regex tolerates alphabetically-sorted toYaml output (effect before key), uncommented_has correctly ignores the # ... never --retry-all-errors comment, and the read-only-mounts split matches the new mount list.

Findings

1. (Medium) dns-test runtime tooling is not exercised by the guard — the two live-cluster test-plan items are still unchecked.
See inline comment at node-health.yaml:321. The dns container image changed from helmTest.dnsImage (busybox) to helmTest.image (curlimages/curl), and the test now relies on getent ahostsv4 + awk + sleep from that image. The guard only greps the rendered YAML for the literal string; it cannot catch a missing/misbehaving getent ahostsv4 in the runtime image. This is the single highest-risk unverified change in the PR — worth completing the two unchecked helm test runs (or a container-level getent smoke check) before merge.

2. (Low–Medium) curl -4 now forces IPv4 for the kube-API and Service probes, a regression risk on IPv6-only / dual-stack clusters.
See inline comment at node-health.yaml:36. The AAAA-stall rationale is legitimate, but the -4 wrapper applies to every curl in the health-test script including kget() to kubernetes.default.svc. On clusters where the API Service resolves to an IPv6 ClusterIP, these probes go from slow to guaranteed failure. Scoping -4 to the specific proxy/DNS hostname probes (or falling back to IPv6 when no A record exists) would avoid the regression.

3. (Low) helmTest.image now has a hard getent/awk contract for the dns-test.
The image swap means any operator who overrides helmTest.image with a minimal image (a common thing to do, since it was previously only used for the curl-based checks) will silently break the dns-test at runtime. It's documented in the README ("Override helmTest.image with curl+sh+awk+getent"), but the failure mode is a red helm test with a generic "could not resolve ... (A)" message, which is hard to diagnose as a missing applet. A one-line guard assertion that the dns container still uses an image containing a known-good tag pattern would help, though the guard can't validate image contents either.

4. (Info) node-runtime-test hardening is sound but only render-verified.
runAsUser/runAsGroup: 0 + readOnlyRootFilesystem: true + drop: ["ALL"] + all mounts readOnly is consistent with the read-only assertions (test -e/-d/-S, no writes), and the root pin prevents a non-root image override from silently failing stat() on root-owned hostPaths — a clean resolution of the original #1272 blocker. As with #1, the guard can't verify the busybox container actually runs these assertions under the hardened securityContext on a real tainted node.

Notes that checked out clean

• README edits are accurate (admin port via Service, Service FQDN for the CoreDNS rewrite, placement/probe/dns contract).
• helmTest.dnsImage is now only used by node-runtime-test, matching the README.
• --retry-connrefused without --retry-all-errors correctly avoids re-drilling HTTP 4xx, and test "$status" = "200" captures the final attempt's code.
• The dataplane / reachability check stays in the separate health-test pod (it tolerates the 400 via -sS without -f), so coverage is not lost by re-pointing proxy-control-test.

Bottom line: approve pending the two unchecked helm test verification items (control-plane-tainted single-node and multi-node-with-compute), and consider the IPv4-only and helmTest.image contract notes above as minor follow-ups rather than blockers.